-
-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AdditionalExpenses and ContactTopicAnswer controllers & policies #6060
AdditionalExpenses and ContactTopicAnswer controllers & policies #6060
Conversation
fea791a
to
bbaf3dd
Compare
@@ -191,6 +194,9 @@ | |||
end | |||
resources :case_court_orders, only: %i[destroy] | |||
|
|||
resources :additional_expenses, only: %i[create destroy], | |||
constraints: lambda { |req| req.format == :json } | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did these as root-level resources. They could be case_contact member resources (nested), as they can't exist without a contact parent. I tend to error on the side of not nesting resources... and it was easier. Can move if need be.
The constraints for :json was new to me. Made sense for the no-view / api style endpoint.
bbaf3dd
to
cf46fac
Compare
cf46fac
to
f09a6b4
Compare
Hey! @thejonroberts Trying to review this PR, mainly trying to get my head about the autosave json stuff. Could you explain this part a bit more (from the big PR):
If I understand you correctly you are saying on every form update(?) we create new contact topic answers and new additional expenses. Doesn't the existing autosave just submit the form in the background with a debounce? It works with both notes and contact topic answers. What is breaking that behavior here? Did you explore any other ways of retaining the autosave without the additional controllers? What were the issues? |
@elasticspoon the relevant bits of the old flow were:
Step 2 is the only step with autosave in effect. Since topics were already selected in step 1, you aren't adding new (non-persisted) ContactTopicAnswers, only changing the value of existing answers. (I don't think removing was an option, you could just sort of ignore the topics you didn't want to elaborate on). Therefore autosave can submit & re-submit the entire form, everything is already in the database, and it's update only. Expenses/Step 3 doesn't autosave, so you can add/remove as much as you want without hitting the database until submission. In the new, all-in-one form, you run into issues autosaving the form:
Put simply, the form needs to be kept in sync with the database when associated records are created or destroyed. The form needs to know IDs of created records and when a record has been deleted, so that autosave can work. The delete could probably be worked around / rescued somehow in the controller. But the Add problem requires the form to know IDs before it tries to autosave any database records. The only other approach I thought of was something like: # in update action method:
@case_contact.contact_topic_answers.destroy_all
@case_contact.additional_expenses.destroy_all
@case_contact.update(case_contact_params) Deleting all the records just seems wrong... especially before an update... seemed too risky to me, if update fails, what then? So I figured make the Add and Delete buttons a database-persisted action via these controllers, plus javascript (other PR) to update the form with the new IDs or removing deleted entries entirely (so they do not make a second destroy request). |
@elasticspoon it seems like a good use of a turboframe - just replace the entire form on autosave.... but I worried about the response replacing an input as the user is interacting with it! |
f09a6b4
to
d3418d2
Compare
What you are saying makes sense. Thanks for the detailed explanation.
I guess you could wrap it in a transaction, but with it happening as often as it does, that might be an issue. |
d3418d2
to
ba5069a
Compare
Closing as #6048 has gotten some feedback and is almost ready for review. |
What github issue is this PR for, if any?
Preparation for Case Contact Form re-design - #6048
Creates policy and controllers for AdditionalExpenses and ContactTopicAnswers. These resources will be used by frontend to persist/delete nested form items as they are added & removed - which is why only create/destroy actions exist in the controllers. This is necessary so that autosave of the form during topic answer (note) input does not re-create a new note (or try to delete a previously deleted note) every time it runs.
Previously, a blank ContactTopicAnswer would be created in the first details step per selected topic... so each answer during notes step would already have a persisted CTA to update during autosave. Expenses were not affected since autosave only happened during notes step.
Separated this stuff out to make for easier review.